Skip to content

Fix delivery_whitelist regex #5676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 22 commits into from
Closed

Fix delivery_whitelist regex #5676

wants to merge 22 commits into from

Conversation

gonzalovilaseca
Copy link
Contributor

No description provided.

@@ -140,11 +140,11 @@ by adding the ``delivery_whitelist`` option:
delivery_whitelist:
# all email addresses matching this regex will *not* be
# redirected to dev@example.com
- "/@specialdomain.com$/"
- "/@specialdomain[.]com$/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imho we should rather escape the period with a backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, evaluating a character class is more expensive than evaluating a single character.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gonzalovilaseca although I can't provide a solid reasoning for this, I agree with @xabbuh here. When escaping just one character, I find it "better" to use the backslash instead of the other notation:

- "/@specialdomain[.]com$/"

- "/@specialdomain\.com$/"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will update PR

@gonzalovilaseca gonzalovilaseca deleted the patch-2 branch September 9, 2015 14:20
@gonzalovilaseca
Copy link
Contributor Author

Moved to #5684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants